Don't allow {} to refer to implicit captures in format_args. #93394

m-ou-se · 2022-01-27T22:52:14Z

Fixes #93378

rust-highfive · 2022-01-27T22:52:17Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

CAD97 · 2022-01-28T10:36:07Z

This should imho probably also add an explicit test for the println!("{a} {}", a=a) case, to document it at least.

CAD97 · 2022-01-28T10:53:17Z

compiler/rustc_builtin_macros/src/format.rs

@@ -369,7 +377,7 @@ impl<'a, 'b> Context<'a, 'b> {

        let count = self.pieces.len()
            + self.arg_with_formatting.iter().filter(|fmt| fmt.precision_span.is_some()).count();
-        if self.names.is_empty() && !numbered_position_args && count != self.args.len() {
+        if self.names.is_empty() && !numbered_position_args && count != self.num_args() {


Normally this (positive) branch would result in a message 1 positional argument in format string, but no arguments were given, whereas now adding an implicit capture gets invalid reference to positional argument 0 (no arguments were given). Ideally this wording would be kept, even with implicit args; I think the proper check would then be self.names.len() == self.num_captured_args && !numbered_position_args && count != self.num_args()?

Then the let count = above would also have to be adjusted to exclude all placeholders that refer to captures, which is a bit more complicated.

I agree the diagnostics here can be improved when there's captured arguments involved, but I rather leave that to a separate change that can fix all those cases properly.

m-ou-se · 2022-02-02T20:06:09Z

Marked as beta-accepted per discussion in the libs api team just now.

estebank · 2022-02-02T22:55:26Z

r=me, but we really want a crater run of this to make sure we're not regressing here and am slightly uncomfortable with a beta backport (but it's not my call)

estebank · 2022-02-02T22:55:43Z

src/test/ui/fmt/format-args-capture-issue-93378.stderr

+error: invalid reference to positional argument 0 (no arguments were given)
+  --> $DIR/format-args-capture-issue-93378.rs:9:23
+   |
+LL |     println!("{a:.n$} {b:.*}");
+   |               ------- ^^^--^
+   |               |          |
+   |               |          this precision flag adds an extra required argument at position 0, which is why there are 3 arguments expected


This output needs some work to make it clearer :(

Yes, there's quite a few cases right now where this macro produces suboptimal diagnostics.

We have plans to change how fmt::Arguments is implemented/represented, which would require a lot of changes to the format_args builtin macro. That would probably be the best time to just rewrite the macro and the way it produces diagnostics.

m-ou-se · 2022-02-03T09:31:59Z

we really want a crater run of this to make sure we're not regressing here

We could run this PR through crater, but I suppose the beta crater run is good enough to catch any problems.

m-ou-se · 2022-02-03T09:36:58Z

Looks like the crater queue is almost empty, so we can quickly run this through crater to be sure:

@craterbot check

craterbot · 2022-02-03T09:36:59Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

m-ou-se · 2022-02-03T09:40:58Z

@bors try

bors · 2022-02-03T09:41:08Z

⌛ Trying commit cef9b47 with merge 1b0139f5bdae7328e6d232947a8112333bb44eeb...

m-ou-se · 2022-02-03T09:52:50Z

I was told crater needs some maintenance after the current experiment finishes, so let's hold off until that's done.

@pietroalbini can you start a check run here once you're done with that? Thanks.

bors · 2022-02-03T10:59:18Z

☀️ Try build successful - checks-actions
Build commit: 1b0139f5bdae7328e6d232947a8112333bb44eeb (1b0139f5bdae7328e6d232947a8112333bb44eeb)

apiraino · 2022-02-03T17:31:36Z

T-compiler discussion about beta-backport approval on Zulip

Also, stable backport nomination discussed and approved (Zulip), in particular that this backport should be included in a point release, but does not motivate one.

@rustbot label +stable-nominated +stable-approved

craterbot · 2022-02-04T02:23:40Z

🚧 Experiment pr-93394 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2022-02-06T07:34:56Z

🎉 Experiment pr-93394 is completed!
📊 2 regressed and 4 fixed (210500 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

m-ou-se · 2022-02-07T12:04:00Z

Just two failures in the crater run. Both of them are unrelated: a build script failing to download some file.

@bors r=estebank

bors · 2022-02-07T12:04:02Z

📌 Commit cef9b47 has been approved by estebank

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

Rollup of 13 pull requests Successful merges: - rust-lang#88313 (Make the pre-commit script pre-push instead) - rust-lang#91530 (Suggest 1-tuple parentheses on exprs without existing parens) - rust-lang#92724 (Cleanup c_str.rs) - rust-lang#93208 (Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0) - rust-lang#93394 (Don't allow {} to refer to implicit captures in format_args.) - rust-lang#93416 (remove `allow_fail` test flag) - rust-lang#93487 (Fix linking stage1 toolchain in `./x.py setup`) - rust-lang#93673 (Linkify sidebar headings for sibling items) - rust-lang#93680 (Drop json::from_reader) - rust-lang#93682 (Update tracking issue for `const_fn_trait_bound`) - rust-lang#93722 (Use shallow clones for submodules managed by rustbuild, not just bootstrap.py) - rust-lang#93723 (Rerun bootstrap's build script when RUSTC changes) - rust-lang#93737 (bootstrap: prefer using '--config' over 'RUST_BOOTSTRAP_CONFIG') Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

[beta] Backports Backports of: * rust-lang#92611 — Add links to the reference and rust by example for asm! docs and lints * rust-lang#92983 — Update Linux runners to Ubuntu 20.04 * rust-lang#93081 — Update LLVM submodule * rust-lang#93394 — Don't allow {} to refer to implicit captures in format_args. * Cargo: * rust-lang/cargo#10377 — Remove strip = "off" (and undocumented strip = "n"/strip = "no")

rust-highfive assigned estebank Jan 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2022

m-ou-se mentioned this pull request Jan 27, 2022

Named format arguments introduce implicit positional arguments #93378

Closed

m-ou-se added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 27, 2022

This comment has been minimized.

Sign in to view

m-ou-se added 3 commits January 28, 2022 00:20

Don't allow {} to refer to implicit captures in format_args.

9b8e4c6

Add test for format args capture bug.

fb2d530

Extend format-args capture test.

cef9b47

m-ou-se force-pushed the fix-93378 branch from 2de8557 to cef9b47 Compare January 27, 2022 23:20

CAD97 mentioned this pull request Jan 28, 2022

Named format arguments can be used as positional #93415

Closed

CAD97 reviewed Jan 28, 2022

View reviewed changes

ehuss mentioned this pull request Jan 30, 2022

String interpolation filling in value instead of compile time error #93478

Closed

m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 2, 2022

estebank reviewed Feb 2, 2022

View reviewed changes

apiraino added stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Feb 3, 2022

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2022

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 6, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2022

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 7, 2022

Rollup merge of rust-lang#93394 - m-ou-se:fix-93378, r=estebank

ad3309d

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

m-ou-se mentioned this pull request Feb 7, 2022

Rollup of 13 pull requests #93736

Closed

m-ou-se mentioned this pull request Feb 7, 2022

Rollup of 13 pull requests #93738

Merged

bors merged commit 4445a8f into rust-lang:master Feb 7, 2022

rustbot added this to the 1.60.0 milestone Feb 7, 2022

matthiaskrgr mentioned this pull request Feb 8, 2022

error in format string : unreachable!("foo {bar,baz}"); no longer compiles #93761

Closed

Urgau mentioned this pull request Feb 8, 2022

Fix invalid special casing of the unreachable! macro #93179

Merged

ehuss pushed a commit to ehuss/rust that referenced this pull request Feb 10, 2022

Rollup merge of rust-lang#93394 - m-ou-se:fix-93378, r=estebank

120e33f

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

ehuss mentioned this pull request Feb 10, 2022

[beta] Backports #93846

Merged

ehuss modified the milestones: 1.60.0, 1.59.0 Feb 10, 2022

ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 10, 2022

Mark-Simulacrum removed stable-nominated Nominated for backporting to the compiler in the stable channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allow {} to refer to implicit captures in format_args. #93394

Don't allow {} to refer to implicit captures in format_args. #93394

m-ou-se commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

This comment has been minimized.

CAD97 commented Jan 28, 2022

CAD97 Jan 28, 2022

m-ou-se Jan 28, 2022

m-ou-se commented Feb 2, 2022

estebank commented Feb 2, 2022

estebank Feb 2, 2022

m-ou-se Feb 3, 2022

m-ou-se commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

craterbot commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

bors commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

bors commented Feb 3, 2022

apiraino commented Feb 3, 2022

craterbot commented Feb 4, 2022

craterbot commented Feb 6, 2022

m-ou-se commented Feb 7, 2022

bors commented Feb 7, 2022

Don't allow {} to refer to implicit captures in format_args. #93394

Don't allow {} to refer to implicit captures in format_args. #93394

Conversation

m-ou-se commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

This comment has been minimized.

CAD97 commented Jan 28, 2022

CAD97 Jan 28, 2022

Choose a reason for hiding this comment

m-ou-se Jan 28, 2022

Choose a reason for hiding this comment

m-ou-se commented Feb 2, 2022

estebank commented Feb 2, 2022

estebank Feb 2, 2022

Choose a reason for hiding this comment

m-ou-se Feb 3, 2022

Choose a reason for hiding this comment

m-ou-se commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

craterbot commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

bors commented Feb 3, 2022

m-ou-se commented Feb 3, 2022

bors commented Feb 3, 2022

apiraino commented Feb 3, 2022

craterbot commented Feb 4, 2022

craterbot commented Feb 6, 2022

m-ou-se commented Feb 7, 2022

bors commented Feb 7, 2022